Skip to content

Thread parking for Kotlin/Common #498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
May 22, 2025
Merged

Thread parking for Kotlin/Common #498

merged 9 commits into from
May 22, 2025

Conversation

bbrockbernd
Copy link
Collaborator

@bbrockbernd bbrockbernd commented Dec 19, 2024 •
edited
Loading

Allows to pause and resume thread execution. On native platforms it is based on pthread_cond_wait, on JVM it uses LockSupport.

Threads can be pre unparked (calling unpark before park cancels the parking operation). And thread can be parker with a timeout.

Suspend the current thread by calling:

ParkingSupport.park(timeout: kotlin.time.Duration)
// or
ParkingSupport.parkUntil(deadline: TimeMark)
// or park without timout
ParkingSupport.park(kotlin.time.Duration.INFINITE)

Resume a thread by calling:

ParkingSupport.unpark(handle: ParkingHandle)

Get current thread reference by calling:

val handle = ParkingSupport.currentThreadHandle()

@bbrockbernd bbrockbernd marked this pull request as ready for review December 19, 2024 11:10
dkhalanskyjb
dkhalanskyjb reviewed Jan 16, 2025
View reviewed changes
dkhalanskyjb
dkhalanskyjb reviewed Jan 29, 2025
View reviewed changes
@bbrockbernd bbrockbernd force-pushed the native-thread-parking branch from b82d7aa to 22d9ca4 Compare January 31, 2025 13:34
@dkhalanskyjb dkhalanskyjb self-requested a review January 31, 2025 14:42
@dkhalanskyjb dkhalanskyjb self-requested a review February 6, 2025 10:59
@bbrockbernd bbrockbernd mentioned this pull request Feb 7, 2025
Open
@dkhalanskyjb dkhalanskyjb self-requested a review February 10, 2025 14:49
dkhalanskyjb
dkhalanskyjb reviewed Feb 10, 2025
View reviewed changes
@dkhalanskyjb dkhalanskyjb self-requested a review February 10, 2025 15:18
dkhalanskyjb
dkhalanskyjb requested changes Feb 13, 2025
View reviewed changes
dkhalanskyjb
dkhalanskyjb requested changes Feb 19, 2025
View reviewed changes
dkhalanskyjb
dkhalanskyjb reviewed Feb 24, 2025
View reviewed changes
atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/parking/TimeArithmeticTests.kt Outdated
val nanos = Random.nextLong()
val updatedTime = currentTimeInSeconds.addNanosToSeconds(nanos)
if (nanos > 0) assertTrue { updatedTime > currentTimeInSeconds }
if (nanos < 0) assertTrue { updatedTime < currentTimeInSeconds }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably makes more sense to assert in the code that the negative number of nanos never even enters addNanosToSeconds. No reason to test the code path that will never be taken.

atomicfu/src/nativeMain/kotlin/kotlinx/atomicfu/parking/PosixParkingDelegator.kt Outdated
internal inline fun callAndVerifyNative(vararg expectedReturn: Int, block: () -> Int): Int {
val returnStatus = block()
if (expectedReturn.all { it != returnStatus })
throw IllegalStateException("Calling native, expected one return status of ${expectedReturn.joinToString(", ")}, but was $returnStatus")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also print the contents of https://www.man7.org/linux/man-pages/man3/errno.3.html

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/parking/TimeArithmeticTests.kt Outdated
val currentTimeInInt = currentTimeInSeconds.toInt()
val nanos = Random.nextLong()
val updatedTime = currentTimeInInt.addNanosToSeconds(nanos)
if (nanos > 0) assertTrue { updatedTime > currentTimeInSeconds && updatedTime <= Int.MAX_VALUE }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A stronger version of the check: if the value does not clamp, the (updatedTime - currentTime) == nanos / 1_000_000_000; if the value clamps, it's because as a Long value, it would exceed an Int. This describes what we want from addNanosToSeconds more precisely. As is, when I write internal fun Long.addNanosToSeconds(nanos: Long): Long = this + nanos / 2_000_000_000, for example, no tests fail.

dkhalanskyjb
dkhalanskyjb requested changes Apr 25, 2025
View reviewed changes
atomicfu/src/androidNativeMain/kotlin/kotlinx/atomicfu/locks/PosixParkingDelegator.kt
actual fun createRef(): ParkingData {
val mut = nativeHeap.alloc<pthread_mutex_t>().ptr
val cond = nativeHeap.alloc<pthread_cond_t>().ptr
val attr = nativeHeap.alloc<pthread_condattr_tVar>().ptr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is irritatingly close to pthread_condattr_t, which is used by the other Unix-like platforms, but after several attempts, I did not manage to find an incantation that would allow avoiding the code duplication for androidNativeMain.

@fzhinkin

This comment was marked as resolved.

Sign in to view
@fzhinkin
Copy link
Collaborator

There are small formatting issues here and there (like trailing spaces), please run the "Code/Reformat Code" in IJ (you can perform a bulk cleanup by selecting all the necessary files and then running the reformatting).

fzhinkin
fzhinkin requested changes Apr 28, 2025
View reviewed changes
Copy link
Collaborator

@fzhinkin fzhinkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! I left a few comments regarding tests and documentation, but overall it looks good.

@bbrockbernd bbrockbernd changed the base branch from master to develop April 29, 2025 10:08
@bbrockbernd bbrockbernd force-pushed the native-thread-parking branch from 519e59d to 0d9ea07 Compare April 29, 2025 11:59
@bbrockbernd bbrockbernd requested a review from dkhalanskyjb May 13, 2025 07:41
@bbrockbernd bbrockbernd force-pushed the native-thread-parking branch from 809689c to 4cd091b Compare May 13, 2025 07:45
fzhinkin
fzhinkin reviewed May 19, 2025
View reviewed changes
Copy link
Collaborator

@fzhinkin fzhinkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few nits, but otherwise the change looks good.

@bbrockbernd bbrockbernd requested a review from fzhinkin May 20, 2025 15:43
dkhalanskyjb
dkhalanskyjb approved these changes May 21, 2025
View reviewed changes
Copy link

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, what next? @ndkoval, do you also want to review this, or can we merge the PR?

@ndkoval
Copy link
Member

ndkoval commented May 21, 2025

@dkhalanskyjb please merge the PR.

@fzhinkin fzhinkin merged commit 82964af into develop May 22, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndkoval ndkoval ndkoval approved these changes

@fzhinkin fzhinkin fzhinkin approved these changes

@dkhalanskyjb dkhalanskyjb dkhalanskyjb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bbrockbernd @fzhinkin @ndkoval @dkhalanskyjb